-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Port to Python 3 #10
Port to Python 3 #10
Conversation
Tested Collaboration on the branch python3, logs are clean for the sharer and is same as master, but got this traceback for the joiner:
What perplexed me was the line: |
Do remember to restart Sugar. At the moment Sugar does not detect change to |
When you say " Collaboration does not include turns so joiner can only join the activity but treated as two separate games. Collaboration makes no sense if there are no turns." do you mean the game state isn't shared? |
@chimosky Yes, also I had seen the code, it does not include "having turns" , like paths/ memorize/ sugarchess defines a variable and stores boolean value to decide who plays when the game board is shared |
Some activities were written by copying the source code of another activity and hacking on it until it worked well enough. That can cause activities that seem to have a collaboration feature but don't. Do you think this might be that scenario? |
Well yes, because all one can do is join the activity. |
Thanks. I'm not familiar with the activity user experience or data model. Best I've found is app store narrative. I've not worked on the activity before. In the repository commit history, a133c0a was a widespread change to how collaboration worked. Is it possible that this change broke collaboration? After resolving the above, do you think it makes sense for the activity to support collaboration? |
It mentions: "The fourth game prompts the user to identify the image which had not appeared in the grid. This time, the images differ by shape and color. Again, it starts easy, with just three images to remember, but gets very challenging as the number of images increases."
This may or may not be the case, I need to test the repository at that point to know for sure. On a quick look, I don't see a variable that acts like a turns variable in the code that was changed.
Honestly, if there is no score board, the collaboration doesn't make sense in this activity (there is no incentive for a child to play more attentively and give the correct answer). It is like playing an individual game but just waiting for the other person to take their turn. The activity is just like a set of questions to be answered and it makes no difference to the child whether he has to answer all the questions or have alternate questions answered by the other player. |
Thanks. Tested abf1183.
I agree it doesn't make sense to have collaboration. In looking at history and source code I found "dotlist" was in the metadata, and I remember seeing that in another activity. A search shows where else it can be found. So I constructed a timeline of the activities that mention it;
Also related to sugarlabs/cookie-search-activity#18. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JuiP you can cherry-pick 051dd9c. I have solved
A Pango-Warning is seen after ff4f92c
Invalid UTF-8 string passed to pango_layout_set_text()
And also after solving I didnt get again-
After selecting the correct object, master displays a smile emoji but after port to python3 seen as a question mark. Still haven't found the cause
For reference I looked at GNOME DEVELOPERS page and found out that second argument taken by set_text (length) is maximum length of text , in bytes. And -1 indicates that the string is nul-terminated and the length should be calculated. So I tried using -1 there as it is also used in ruler and it worked.Thanks.
I agree.
Yes noticed this too, removing the line wrap set on the label fixes it.
@JuiP You should update |
6eb5e60
to
3e21299
Compare
Thanks @Saumya-Mishra9129 done in 3e21299 |
About collaboration, I think it is better to keep the status of collaboration as it is at the moment. Later, maybe we could add scoreboard feature so that it is more meaningful. |
I wonder if it might be fun to have the players have to cooperate to decide on an answer... The game is quite challenging as it progresses. |
The next release will be the first release to let the activity run on Sugar again after the Python 3 transition, so for the moment I think it is fine for collaboration to be functionally unchanged; it doesn't work in the latest release, so it is fine if it doesn't work in the next release. I don't think the collaboration code should be ported. So I suggest ripping it out. If it makes sense to add collaboration, that can be in another release. I don't think it will save any time to keep broken collaboration code in the source. See branch inhume-sharing for commits against master that remove collaboration. I was able to rebase your latest commit as 5533c49 after resolving a few conflicts. If you wish to proceed on this basis, you might try rebasing as I did, or at least compare 5533c49 against master. |
Invalid UTF-8 string is passed to pango_layout_set_text() is fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for testing @srevinsaju! :)
Planned for a later release for now |
@JuiP Thanks for the quick reply. In that case; I would be more glad if you change this to self.max_participants = 1 This would disable the collaboration button too; This removes the confusion if collaboration is implemented or not; from the users' side |
@srevinsaju It has been done in 666da4c. You can see if you try using that button, then it gets disabled. If you are referring to your PR in iknowSriLanka then the button implementation was a little different than in this activity. |
No, I am not talking about my implementation; setting |
@JuiP, although setting
at the self.max_participants = 4 on L78 https://github.com/JuiP/recall/blob/python3/RecallActivity.py#L75-L80 def _setup_toolbars(self, have_toolbox):
""" Setup the toolbars. """
self.max_participants = 4
toolbox = ToolbarBox() |
@srevinsaju Thanks for noticing! I have done it in 3f7a927 |
@@ -54,6 +44,7 @@ class RecallActivity(activity.Activity): | |||
def __init__(self, handle): | |||
""" Initialize the toolbars and the game board """ | |||
super(RecallActivity, self).__init__(handle) | |||
self.max_participants = 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JuiP, I guess; we could remove this instance of self.max_participants
if its possible to. then this PR would be perfect!
Review, thanks. |
Tested; Apart from the two issues I didnot notice differences in python3 branch and master.
Differences between this branch and master-
Invalid UTF-8 string passed to pango_layout_set_text()
Observed this before port to python3
In master:
After porting ; In python3 branch:
@quozl @chimosky Please review changes!